AVRO-3403: Create and use ANTLR to parse IDL files #1588
Conversation
opwvhk
force-pushed
the
AVRO-3403-migrate-JavaCC-to-ANTLR
branch
2 times, most recently
from
5d85cd1 to
36779cc
Compare
|
Added a change to the grammar (sorry for the way in which I did...) such that it the generated code can also work on Python. |
There was a problem hiding this comment.
I didn't review yet the new IdlReader class and few related classes, mostly because it is big and I have zero experience with ANTLR. I will try do to it soon!
Apart from my comments there are also some CodeQL comments in the diff view which might be addressed.
lang/java/archetypes/avro-service-archetype/src/main/pom/pom.xml
Outdated
Show resolved
Hide resolved
lang/java/maven-plugin/src/main/java/org/apache/avro/mojo/IDLProtocolMojo.java
Outdated
Show resolved
Hide resolved
lang/java/tools/src/main/java/org/apache/avro/tool/IdlToSchemataTool.java
Outdated
Show resolved
Hide resolved
opwvhk
force-pushed
the
AVRO-3403-migrate-JavaCC-to-ANTLR
branch
from
ce7b25f to
11fa739
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found 14 potential problems in the proposed changes. Check the Files changed tab for more details.
opwvhk
force-pushed
the
AVRO-3403-migrate-JavaCC-to-ANTLR
branch
from
0538c89 to
90694ab
Compare
|
Hi! Our tool BreakBot found that this pull request introduces 3 breaking changes and they appear to impact 5 of your clients. You can find the full BreakBot report in our fork repository: report for PR#1588. We hope this information is valuable to you, and apologize otherwise. If you're willing to help, we would kindly ask for your help to fill in a 5-minutes survey about the report. Your feedback will help us improve the tool and provide a better experience for users in the future! |
|
The BreakBot report marks the Set aside that these are not actually breaking changes, they were added on purpose to signal a migration is needed for future compatibility. |
|
You are right. We include the impact of |
opwvhk
force-pushed
the
AVRO-3403-migrate-JavaCC-to-ANTLR
branch
from
1a1bbe1 to
78ffe6c
Compare
Because this is a shared location, which allows the grammar to be reused for e g. a Python implementation. The reason to migrate to ANTLR is partly because it is supported, but also because it supports multiple languages. |
This commit also copies several utilities from avro-compiler.
The build on Travis compiles files in a different order, causing the tested new log lines to appear in the 'wrong' order. This removes these new log lines.
Improved the code thanks to good review comments.
Integer/Long parsing now also allows the hexadecimal and octal syntax that the grammar allows.
opwvhk
force-pushed
the
AVRO-3403-migrate-JavaCC-to-ANTLR
branch
from
78ffe6c to
b76d39d
Compare
While merging, clarified the use of a source file for modofication detection.
|
After resolving the conflicts, I'd like to merge this one and move on to PR #1589 (making the IDL syntax a full |
This large change encompasses a new avro-idl module, a shared ANTLR grammar to allow implementations in other languages, and uses (copies of) all existing IDL tests.
What is the purpose of the change
Implement AVRO-3403: Migrate JavaCC to ANTLR
The implementation uses a shared ANTLR grammar, without actions or
predicates. This guarantees ANTLR can create usable parsers in any
language it supports. To implement IDL support for other languages, the
Java code to port is the class
IdlReader.The IDL reader using the ANTLR generated parser replaces the JavaCC
parser in the maven plugin and in the tools. The JavaCC parser has been
deprecated, but not removed.
Verifying this change
This change is huge, but consists roughly of these types of changes:
IDLReaderclass in a new Javaavro-idlmoduleavro-compilerIDLReaderclass instead of the (now deprecated)IDLclassNote that reviews become a lot easier by reviewing per commit.
Documentation
no)not applicable/ docs / JavaDocs /not documented)